api: support rate limit action based on cidr match #42845
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
agrawroh
left a comment
agrawroh
left a comment
There was a problem hiding this comment.
Just curios, why can't we use the existing Dynamic Metadata for this? It should be trivial to write Dynamic Metadata for Remote Address and use that instead right?
|
@agrawroh the challenge here is not to apply rate limits to remote address ,in fact we already have the RemoteAddress rate limit action for this and can also use GenericKey with CEL without any dynamic metadata. But the usecase here is to apply different rate limits to remote addresses matching on some CIDRs. See the referred issue #36442 and also the related envoyproxy/gateway#4385 issue. |
rudrakhp
force-pushed
the
api_cidr_matcher
branch
from
75d5207 to
42e82a7
Compare
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for the contribution and some comments are added.
Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
rudrakhp
force-pushed
the
api_cidr_matcher
branch
from
42e82a7 to
82f2214
Compare
|
/retest |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks!
I think that matching is a good feature to have, I just wonder whether it could be more generic.
|
|
||
| // [#not-implemented-hide:] | ||
| // Rate limit on remote address match. | ||
| RemoteAddressMatch remote_address_match = 13; |
There was a problem hiding this comment.
I think this new field is confusing because there are already remote_address and remote_address_match. I think calrifying the reasoning behind this feature is the first step. I'm not an expert in this domain, but I would suggest one of the following:
- consider adding options to
RemoteAddressandMaskedRemoteAddressthat enables the matching (the reasoning behind this is that I think that matching is more generic and orthogonal to whether the matched object is a remote address, a request header, etc). - See if this should be a typed-extension (using the
extensionfield).
cc @wbpcode due to the comment in #36442
There was a problem hiding this comment.
@adisuissa thanks for the comment. This is supposed to handle all the remote address matching usecases in a generic way by leveraging CEL and substitution formatting in descriptor_value. For example:
- Using
%DOWNSTREAM_REMOTE_ADDRESS%here will handle the existingRemoteAddressusecase with CIDR matching. - We might need to introduce formatter like
%MASKED_DOWNSTREAM_REMOTE_ADDRESS(len)%to handleMaskedRemoteAddressusecase with CIDR matching. See feat: add formatters for masked IP addresses #42969 .
@wbpcode is aligned with the RemoteAddressMatch approach (see comments above), if the name is confusing I will think of some alternatives.
There was a problem hiding this comment.
^ bump @adisuissa @wbpcode
How does remote_address_range_match / remote_address_value_match sound instead of remote_address_match?
There was a problem hiding this comment.
@wbpcode is the expert on substitution formatters. I'm just presenting my thoughts on design and combination of features.
There was a problem hiding this comment.
Sure, thanks for your inputs!
<!-- !!!ATTENTION!!! If you are fixing *any* crash or *any* potential security issue, *do not* open a pull request in this repo. Please report the issue via emailing envoy-security@googlegroups.com where the issue will be triaged appropriately. Thank you in advance for helping to keep Envoy secure. !!!ATTENTION!!! For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/main/PULL_REQUESTS.md) !!!ATTENTION!!! Please check the [use of generative AI policy](https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md?plain=1#L41). You may use generative AI only if you fully understand the code. You need to disclose this usage in the PR description to ensure transparency. --> Commit Message: Add formatters for masked IP addresses Additional Description: Added masked alternatives for supported substitution formatters that return IP addresses today Risk Level: Low Testing: Unit testing Docs Changes: Added the new substitution formatters to docs Release Notes: Yes Platform Specific Features: N/A Related: #42845 (comment) Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>
4 participants
Commit Message: [api] support rate limit action based on cidr match
Additional Description: Proposed API changes to support rate limit actions on remote address match. This includes additions of a new rate limit action
RemoteAddressMatchas well as inclusion of a CIDR matcher in theMaskedRemoteAddressaction.Risk Level: Low (doesn't break/affect existing usecases)
Testing: N/A (API changes only)
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:]
Related #36442
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
Optional API Considerations:
RemoteAddressMatchrate limit action will cater to usecases that need to match on remote address.%MASKED_REMOTE_ADDRESS(prefix_len)%.type.matcher.v3.AddressMatcher. Need to implementinvert_matchin existing usage atFilterStateIpRangeMatcher::match.